dns: add a new RUST-based DNS resolver extension#44090
dns: add a new RUST-based DNS resolver extension#44090agrawroh wants to merge 4 commits intoenvoyproxy:mainfrom
Conversation
e720a35 to
fd827fd
Compare
|
/gemini review |
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
There was a problem hiding this comment.
Code Review
This pull request introduces a new Rust-based DNS resolver extension, Hickory, which is a great addition. The implementation is comprehensive, including the core logic in Rust, the C++ shell, ABI definitions, build system changes, documentation, and tests.
My review has identified two main issues:
- A large, unrelated
TracerABI definition has been included insource/extensions/dynamic_modules/abi/abi.h. This should be removed. - There's a bug in the DNS-over-HTTPS configuration logic in the Rust implementation that prevents using hostnames in DoH URLs.
Apart from these points, the changes look solid. The C++ shell and Rust SDK are well-designed for thread safety and proper resource management.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new Rust-based DNS resolver extension, Hickory, to Envoy. This is a substantial and well-executed feature, including a new dynamic module ABI for DNS resolvers, a Rust SDK for this ABI, the C++ shell for the extension, and the Rust implementation of the resolver logic. The code is well-structured, documented, and comes with comprehensive unit and integration tests. My review identified a critical issue in the new Rust SDK module where FFI functions are not panic-safe. Panics from user code could unwind across the FFI boundary, leading to undefined behavior and process crashes. I have provided comments with code suggestions to wrap the FFI calls with catch_unwind to ensure safety, consistent with other parts of the dynamic module SDK.
b5cc9d9 to
377e75f
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new Rust-based DNS resolver, Hickory, as a dynamic module. This is a significant and well-executed addition, providing modern DNS features like DNS-over-TLS, DNS-over-HTTPS, and DNSSEC. The implementation is thoughtfully split between a C++ shell and a Rust module, communicating via a new, well-documented DNS resolver ABI. The changes are extensive, touching API definitions, build systems, documentation, and adding new implementation and test files. The C++/Rust interaction, particularly the threading and shutdown logic, appears robust. The tests are comprehensive, covering both unit and integration scenarios. I have one suggestion to enhance configuration validation in the Rust module.
af8efcb to
41d196c
Compare
Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com>
mathetake
left a comment
There was a problem hiding this comment.
one thing is that we should have exactly one cargo.toml that produces the .a archive for static Rust-based extensions, otherwise we would end up having multiple copies of Rust stdlibs (or i think the linker will fail once we have multiple objects).
Yeah, I was trying to do that but couldn't make it work. Let me try again and see if I can get it to work. |
Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com>
| srcs = ["hickory_dns.rs"], | ||
| edition = "2021", | ||
| deps = [ | ||
| "//source/extensions/dynamic_modules/sdk/rust:envoy_proxy_dynamic_modules_rust_sdk", |
There was a problem hiding this comment.
so this will still result in the conflict i believe. What i meant in the previous comment was that we should have exactly one rust_static_library in the envoy repo for all the rust based static extensions and then use the symbol prefixing stuff already existing in the dynamic_modules.bzl.
Then, on the extension wiring side, in this case hickory_dns_impl.cc, you can use the dynamic_modules.h API to load the module.
In other words I think we should have the crate at source/dynamic_modules/builtin_extensions (or whatever naming) and having the rust implementations there in a central place (that would be helpful in development too i think regarding language server etc). Then let's have the rust_static_library there so we can ensure only one Rust stdlib as well as any other dependencies would be shared (notably helpful for having the consistent logging library too). After that, you can bring in the rust_static_library here in this extension wiring side
There was a problem hiding this comment.
and then let's add the central crate here
Line 2 in ede8074
…k-dns Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com>
Description
This PR adds a new RUST-based DNS resolver extension to Envoy.
Commit Message: dns: add a new RUST-based DNS resolver extension
Additional Description: Added a new RUST-based DNS resolver extension to Envoy.
Risk Level: Low
Testing: Added Tests
Docs Changes: Added
Release Notes: Added